-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/focus mgmt main menu #2101
Conversation
*/ | ||
setTimeout(() => { | ||
this.$refs.menubutton.$el.focus() | ||
}, 500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it need to be a fixed delay value or can it be done with this.$next()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
500-100ms is a recommended value. setTimeout 0 or this.$next are too short, we have to wait for the Virtual Buffer of the screen reader. This can't be perceived via JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we are using this patterns in multiple places, I wonder if there is a way to make it more Vue-y.
Maybe implement something like this.$next
but call it this.$afterScreenReaderInit
or something so that it becomes self-explanatory ? The latter implementation can then contain whatever value we chose for the delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afterScreenReaderInit
sounds like an actual hook/lifecycle event. I would go with delayForScreenreader
or the like. Emphasising that it is a form of guessing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcus-herrmann your proposal sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PVince81 So this would be eventually a Vue mixin which potentially every component can access, and it its core a wrapper around a setTimeout in order to not need to comment it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcus-herrmann yes, that would be a way.
One question that does come to mind is how can developers know when to use it and when not to. Does this need some experimentation with accessibility test systems or is there a common pattern ? If the latter, it could be included in the design guidelines in some form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PVince81 It's always better to test/experiment with assistive technology, but I can devote a particular page in the design guidelines about screen readers and problems with web-apps (which are, predominantly, DOM-changing, asynchronously loading, stateful constructs, all of which makes them kind of hard to grasp for screen readers). Sending focus to elements in dynamic DOM parts of the application (like this, or after routing) would be just one part of the challenges, accessible notifications, live search and filtering would be others.
@@ -118,6 +118,7 @@ | |||
] | |||
}, | |||
"dependencies": { | |||
"deepmerge": "^4.0.0" | |||
"deepmerge": "^4.0.0", | |||
"inert-polyfill": "^0.2.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. so we could in theory use this attribute for all elements whenever a modal is open. might need to track the info whether a modal is open in the global store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inert does make sense in general when there is need for rendering parts of the DOM inert/inactive/unreachable for assistive technologies/keyboard users. The elements "behind" an open modal are the most prominent use case: https://github.com/WICG/inert/blob/master/README.md
I had a quick test and focus management looks fine for the regular case |
After the click I get a broken cla-assistant.io page and: "Error - There is no CLA to sign for owncloud/phoenix" |
@DeepDiver1975 can you confirm the "no CLA" case ? I also get a rather broken page from the CLA bot when I open https://cla-assistant.io/owncloud/phoenix?pullRequest=2101 |
@DeepDiver1975 Could you point me towards what you mean by dependency conflict? |
Conflict inside of package.json |
💥 Acceptance tests Files failed. Please find the screenshots inside ...https://drone.owncloud.com/owncloud/phoenix/5767/
|
Just was able to sign the CLA. |
@marcus-herrmann @LukasHirt can we move this forward ? I'll need the inert functionality soon |
@PVince81 From my part yes, although I am not sure what exactly is causing the failing build |
@marcus-herrmann please rebase. There were random failures in the past and they will hopefully disappear now. If not I can have a look later on. |
5cea244
to
af52a39
Compare
Add focus management regarding off canvas main nav, #1647
af52a39
to
249b1ba
Compare
@PVince81 Done |
seems the tests would pass. the two failures are due to "npm install" not going through... I've restarted the build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Description
The content of this PR deals with keyboard focus management in the context of the off canvas navigation. It has got to major parts
Related Issue
Motivation and Context
Improve Accessibility: Keyboard navigation is not only crucial for keyboard only users, but all screen reader users
How Has This Been Tested?
Manually by hitting tab key and logging the document.activeElement in the browser's console.
Screenshots (if appropriate):
Types of changes
Checklist:
Open tasks: